Aggregate batteries stats #14
Conversation
There was a problem hiding this comment.
Hey, thanks for the PR and apologies for such late response! Hope you're still around and well :-).
Generally looks good. There is a reason, though, why this has been open for so long (which I never commented on and shame on me). The implemented scenario works fine, as long as batteries are connected in parallel. Which is probably the most common case out there, but it might not always be true. And I have not found a way to make it generic, without introducing complexity in other places (e.g. having to specify a matrix of connections or sth).
Now, I think I am fine with having this method as-is, but would like to see it documented in more detail on what it does exactly. I'd like to try to avoid people using this on in-series connections and wondering why their capacity is getting summed up, etc.
Cheers!
| []*Battery{{State: Empty}, {State: Charging}}, | ||
| &Battery{State: Charging}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I'd say this bracket should go to the previous line, for consistency.
| res.Design += battery.Design | ||
| res.Voltage = math.Max(res.Voltage, battery.Voltage) | ||
| res.DesignVoltage = math.Max(res.DesignVoltage, battery.DesignVoltage) | ||
| res.State = State(math.Max(float64(res.State), float64(battery.State))) |
There was a problem hiding this comment.
Given how the states are ordered, I think it actually makes sense. Sure, there are weird cases like 1 charging 1 discharging, but in such scenario I'd rather know about the discharging part, I think.
Maybe it would be worth introducing specific states for summarize? Not sure ATM.
What I think would be good is have some more test cases for this, e.g. that for Full and Discharging it actually returns Discharging, etc.
Issue #4
Add a
Summarize() (*Battery, error)func to aggregate data from multiple batteries.@KenjiTakahashi not sure tho about some metrics, like
State. I followed the simplest logic - higher (and more important) status overrides others, i.e. if a few batteries are full and one is discharging, status will beDischarging